Adopt Swatinem/rust-cache for faster CI #1676
Conversation
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
sideeffffect
force-pushed
the
rust-cache
branch
from
4f20f21 to
7b610f4
Compare
|
I've noticed that compiling Rust takes a lot of time in CI. Hopefully this will help 🤞 |
sideeffffect
force-pushed
the
rust-cache
branch
2 times, most recently
from
b0ac038 to
6e01f8c
Compare
Signed-off-by: Ondra Pelech <[email protected]>
sideeffffect
force-pushed
the
rust-cache
branch
from
6e01f8c to
c4bdbdc
Compare
|
Hi, thanks for submitting this! In general actually I'm trying to move most things into a container build, and there's some more nontrivial work to enable using this action with that. We're relying today on intermediate cache layers in the container build. Now docker does have support for caching layers to/from GHA but podman doesn't directly. It might not be had to add though. My inclination on compliation speedup here was to look at https://github.com/mozilla/sccache/ |
| with: | ||
| cache-all-crates: true | ||
| save-if: ${{ github.ref == 'refs/heads/main' }} | ||
| lookup-only: ${{ github.ref == 'refs/heads/main' }} |
There was a problem hiding this comment.
Why are you setting lookup-only?
There was a problem hiding this comment.
This will ensure that on main, no cache will be used and everything will be built from scratch.
But if you don't like it, it doesn't have to be there.
There was a problem hiding this comment.
This will ensure that on main, no cache will be used and everything will be built from scratch.
Yes, that's the effect the change has. I'm asking about why we want that effect - it's not the default, so you went to some effort to turn it on.
A github-wide search shows it's definitely used elsewhere but...why?
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| cache-all-crates: true | ||
| save-if: ${{ github.ref == 'refs/heads/main' }} |
There was a problem hiding this comment.
Why? My understanding is that due to how github handles caching we're safe against e.g. malicious PRs.
There was a problem hiding this comment.
This will ensure that caches are saved only on main, not on any other PRs. I think that's enough caching. It may also have some vague security benefits.
But if you don't like it, it doesn't have to be there.
There was a problem hiding this comment.
I think anything that's not the default should have a comment with a rationale. When you say "enough caching" - is that to ensure that we're not evicting caches from git main with pull requests?
There was a problem hiding this comment.
Followup for this in bb0693c which adds comments, let me know if I got that wrong
|
Regarding why change the defaults: if the cache is always created in a pristine environment, there's much higher chance of working around any potential compiler bugs. If the compiler and it's incremental compilation were 100% correct, we wouldn't need to worry about it. But the potential for bugs is always there. This would give us most of the benefits of caching while avoiding potentially nasty problems related to caching. But if you'd rather keep things simple, without these opt-in features, no worries, I'd be happy to remove it. Regarding cashing via containers/layers/images, I don't have a good experience with it. You're in all likelihood much bigger expert then me. But I had trouble with determinism, the layers will have always different hashes, even for the same data; this is caused by timestamps which will continue to change. Thus the only reproducible container tool that I use now is JIB from Google (sbt-jib), it fixes the timestamps to 0 (or 2010-01-01 or something like that), ensuring the whole layer(s) have bit per bit identical data and thus hashes. I hope this helps :) |
No description provided.